Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add CloudSQLMySQLEngine class #7

Merged
merged 8 commits into from
Jan 25, 2024
Merged

feat: add CloudSQLMySQLEngine class #7

merged 8 commits into from
Jan 25, 2024

Conversation

jackwotherspoon
Copy link
Contributor

@jackwotherspoon jackwotherspoon commented Jan 24, 2024 •
edited
Loading

Adding CloudSQLMySQLEngine that will be leveraged by the vector store, docloader and memory APIs.

  • Code complete
  • Lint passing
  • Add integration tests (being done in follow-up PR)

Manually tested the following currently:

from langchain_google_cloud_sql_mysql import CloudSQLMySQLEngine
import sqlalchemy

engine = CloudSQLMySQLEngine.from_instance(
    "my-project", "my-region", "my-instance", "my-database"
)

# currently using engine.engine as CloudSQLMySQLEngine does not have a `connect` method
with engine.engine.connect() as db_conn:
    time = db_conn.execute(sqlalchemy.text("SELECT NOW()")).fetchone()
    print(time[0])

@jackwotherspoon jackwotherspoon self-assigned this Jan 24, 2024
@jackwotherspoon jackwotherspoon changed the base branch from main to staging January 24, 2024 21:04
@jackwotherspoon jackwotherspoon marked this pull request as ready for review January 24, 2024 21:28
@jackwotherspoon jackwotherspoon requested a review from a team as a code owner January 24, 2024 21:28
@jackwotherspoon jackwotherspoon requested review from loeng2023 and totoleon and removed request for a team January 24, 2024 21:29
totoleon
totoleon reviewed Jan 24, 2024
View reviewed changes
src/langchain_google_cloud_sql_mysql/cloud_sql_engine.py Outdated Show resolved Hide resolved
docs/document_loader.ipynb Outdated Show resolved Hide resolved
src/langchain_google_cloud_sql_mysql/cloud_sql_engine.py Outdated
self._region = region
self._instance = instance
self._database = database
self.engine = self._create_connector_engine() if engine is None else engine
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we have some validation check on input argument? For example, if engine is given, we shouldn't take other arguments like project ID and region etc.

We don't have to do it in one CL, but having a TODO comment would be good.

Copy link
Contributor Author

@jackwotherspoon jackwotherspoon Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed engine from __init__ entirely as I have removed from_engine and from_connection_string which were the methods passing in the engine

src/langchain_google_cloud_sql_mysql/cloud_sql_engine.py Outdated Show resolved Hide resolved
@jackwotherspoon
Copy link
Contributor Author

@totoleon ready for another pass

@jackwotherspoon
Copy link
Contributor Author

Merging to unblock any future dev, can make any future comments/suggestions in follow-up PR

@jackwotherspoon jackwotherspoon merged commit 9ff3839 into staging Jan 25, 2024
6 checks passed
@jackwotherspoon jackwotherspoon deleted the CloudSQLEngine branch January 25, 2024 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loeng2023 loeng2023 loeng2023 approved these changes

@totoleon totoleon Awaiting requested review from totoleon

Assignees

@jackwotherspoon jackwotherspoon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jackwotherspoon @totoleon @loeng2023